fix: stale hover prefetch degraded a click to a full page load - #1115
Conversation
…load The intermittent whole-document flash on webjs.dev (#1114), tab spinner included, was the client router consuming a poisoned prefetch entry and then correctly bailing to location.href when the fragment shared no boundary with the live DOM. Two composing defects, both fixed: 1. The prefetcher could prefetch the page the user is ALREADY ON. The producer is a hover's intent timer surviving the navigation: hover a link, click it, the swap lands, then the timer fires and prefetches the destination against the destination's own boundaries. The server correctly answers "you have everything" with a near-empty fragment, which lands in the cache under the destination URL. prefetch() now no-ops when the target is the current page. 2. prefetchTake() ignored the X-Webjs-Have context an entry was fetched against. The server marks the reduced response Vary: X-Webjs-Have; the prefetch cache is a client-side cache of that response and must honor its own vary dimension. An entry whose stored have differs from the live buildHaveHeader() is now discarded, costing one round-trip on a stale hit instead of a full document load on consumption. Why it read as random: the poisoned entry lived under a 30s TTL, hovering again did NOT refresh it (the fresh-entry check skips refetching), a reload cleared the in-page cache, and the next lap re-poisoned it, so the reported cycle alternated clean and flashing laps. Reproduced 6-of-24 clicks before the fix, 0-of-24 after, same randomized hover dwells. Also ships the diagnostic that named the path: devWarnFallback becomes reportFallback, which dispatches a webjs:navigation-fallback event on document in EVERY environment ({ cause, href, willReload }) and keeps the dev-only console warning. The old warning was production-silent, which is why this class of bug was undiagnosable on the deployed site and got misattributed twice. The deploy-mismatch reload paths, which previously had no diagnostic at all, now report too.
7c409a6 to
2029fa9
Compare
Unit (packages/core/test/routing/router-client.test.js): the current-page guard including its path+search keying (a hash-only target is the same document and must be suppressed; a different search is a different page and must still warm), plus prefetchTake refusing a stale-context entry AND still consuming a matching one, so the guard cannot silently disable prefetching altogether. Browser (packages/core/test/routing/browser/prefetch-stale-context.test.js): the reported hover, swap, click sequence in DOM terms, asserting no consumable poison survives it. Real-browser because the entry only matters through buildHaveHeader() reading boundary comments out of a live document.body, and the failure mode is location.href, which linkedom does not model. Passes on Chromium, Firefox and WebKit. The browser test waits on the webjs:prefetch event rather than a bare setTimeout(0): the response body read is genuinely async in a browser, and the timeout version raced and failed intermittently across engines while writing it. Which is a small echo of the bug under test, so it is noted in the helper rather than silently worked around.
Two new public surfaces from #1114, on every doc surface that describes the client router. The prefetch cache is context-keyed, not just URL-keyed. A prefetched fragment is a reduced response varying on X-Webjs-Have, so an entry is a fragment RELATIVE to the DOM it was fetched against; the cache records that context and discards a mismatch on consume. Worth documenting rather than leaving internal, because the observable behaviour is "a prefetch sometimes does not produce a cache hit", which looks like a bug without the reason. webjs:navigation-fallback is genuinely new public API: dispatched on document in ALL environments whenever the router degrades a soft nav, with its cause. Documented alongside webjs:navigation-error, with the full cause list and the willReload flag that separates a click becoming a document load from a background op being dropped. Surfaces: AGENTS.md (the client-navigation paragraph), the skill's client-router-and-streaming reference (prefetch section + a new observing section), and website/app/docs/client-router. The scaffold copies the canonical skill at create time, so generated apps pick the reference up with no template change.
vivek7405
left a comment
There was a problem hiding this comment.
Reposting the inline findings: the previous review's code references were eaten by a shell quoting bug on my side. Summary unchanged and repeated here so this review stands on its own.
My own fix does not hold up. Three real problems, two of them regressions I introduced.
The core mistake: I treated the whole X-Webjs-Have string as the validity key for a cached fragment. It is not. A reduced fragment is anchored at ONE boundary, its own outermost wj:children comment, and it applies to any live DOM that still has that boundary with the same route-key. Confirmed against the real server: have=/:/ returns a fragment anchored at the root, which applies on every page in the app; have=/docs:/docs,/:/ returns one anchored at /docs, which applies only under /docs. So a whole-string comparison discards entries that would have applied perfectly, in the common mobile flow (viewport prefetch warms the navbar, one soft nav, then a tap).
Worse, the same over-strict check fights applyOptimisticLoading(), which deletes the page's own boundary pair before the fetch to drop in a loading skeleton. On any route with a loading.{js,ts} the live have is shorter at consume time than at prefetch time on the SAME page, with no navigation at all, so the entry is discarded every time: a permanent two-requests-per-click regression on exactly the routes that opted into skeletons.
And the headline browser test is vacuous, passing on the unfixed bundle. Tests 1 and 2 in that file are sound; the third proves nothing.
Also: the new event has no test despite being documented as public API on three surfaces, one full-page-load path still does not report it while all three docs claim every path does, and the dev warning now prints "fell back to a full page load" for causes whose whole point is that they suppressed the reload.
The diagnosis and guard 1 stand. Guard 2 needs to check the fragment's ANCHOR, not the whole string.
…e string Review of my own first attempt found the vary check was too strict and introduced two regressions. A reduced fragment is anchored at ONE boundary, the deepest the server short-circuited on, and applies to any live DOM that still offers that boundary with the same route-key. Comparing the whole X-Webjs-Have string instead discarded entries that would have applied: prefetch /docs/x from /, soft-nav to /blog, click, and the root-anchored fragment was thrown away for a round-trip main did not pay. Reachable in the ordinary mobile flow, since viewport is the touch default: warm the navbar, navigate once, tap. Worse, applyOptimisticLoading removes the page's own boundary pair before the fetch to insert a loading skeleton, so on any route with a loading.ts the live have is legitimately SHORTER at consume time than at prefetch time with no navigation at all. Every hover-then-click on such a route paid two requests, permanently. So prefetchTake now reads the fragment's anchor (its first open-boundary comment, via regex, since the full parse happens moments later anyway) and checks membership in buildHaveHeader(), which is the single source of truth for the segment:routeKey format. No anchor found means no constraint, so a future shape change degrades to the old permissive behaviour rather than rejecting everything. The entry is only deleted once a decision is made. Also from the same review: - handleNavigationError's last-resort location.href now reports too. It is a click becoming a document load, and three doc surfaces claim every path reports, so omitting it made the documented analytics recipe undercount. - The dev warning reads willReload instead of hardcoding "fell back to a full page load", which was wrong for the two causes that suppress the reload. - The three doc surfaces are corrected: they described the whole-string rationale, which was the thing that turned out to be wrong. Tests: prefetchAnchor unit coverage including the no-marker degrade path; a table-driven prefetchTake test over root-anchored-still-live, deep-anchored-gone, deep-anchored-still-live; and the new event finally gets coverage (cause + href + willReload, on both a reloading and a non-reloading degradation). The browser test that passed vacuously is replaced: it was short-circuited by the in-flight dedupe rather than the guard, and compared against a location.href the runner never has.
|
Follow-up: the missing fallback path and the event's coverage gap Two things from the review that were not inline findings on a single line, recorded here so the reasoning is on the PR rather than only in the commit.
The event now has tests. It was documented as public API on three surfaces with nothing asserting it fires, which is exactly the shape of gap that let the original bug hide: a diagnostic nobody verifies can be silently broken by a later refactor and nobody notices until the next undiagnosable production incident. Two unit tests now pin On the doc surfaces: all three described the whole-string rationale, which was the part that turned out to be wrong, so they are corrected rather than merely extended. They now say anchor-validated and explain which fragments survive a navigation and which do not, since "my prefetch sometimes does not produce a hit" is otherwise indistinguishable from a bug. |
Second review round found the anchor check was still judging against the
wrong view of the DOM, and that the causal story I had written was wrong in
a way that mattered.
applyOptimisticLoading deletes everything between the chosen slot's markers,
which includes every NESTED boundary comment, and it runs BEFORE the fetch.
So on an app whose only loading.{js,ts} sits at the root, buildHaveHeader()
afterwards reports just the root and every deeper anchor looks dead: no
prefetch is ever consumable on that whole app. The skeleton now captures the
boundary keys before it deletes them, on the state that already threads to
fetchAndApply, and prefetchTake validates against that when present.
The narrative correction, which is the more important half. I had written
that a self-prefetch returns a near-empty fragment anchored at the page's own
boundary, and that not prefetching the current page was therefore the cure.
Both wrong. The server short-circuits on LAYOUT segments and ignores the
page's own boundary entry, so a self-prefetch of /docs/x while holding the
docs layout returns a normal 46KB fragment anchored at /docs:/docs, byte
identical to a legitimate sibling prefetch. It applies fine from a sibling
/docs page and fails only from outside /docs.
So the anchor check is the cure and the current-page guard is not; it removes
one producer and a wasted request (which is #1106's point). The other
producer it does not touch at all: hover a sibling link, never click it, then
leave the section. That path now has its own browser test, because it is the
one that proves which guard is doing the work.
Also from the round: the new navigation-error-unrecoverable cause was missing
from both documented cause lists, so an app filtering on the documented set
would have dropped exactly the path this PR added reporting to; the have
field on PrefetchEntry was dead once validation moved to the anchor and is
removed rather than left with a comment claiming a purpose; and the comment
at the consume site still described the pre-change same-shell model.
vivek7405
left a comment
There was a problem hiding this comment.
Second pass, aimed at the new anchor regex and the event's blast radius. The regex held up under attack (checked against real server output across full and reduced responses, with the framework head, the trailing loading templates, and the two docs pages that literally document these markers, which emit them escaped). What did not hold up is the story I told about WHY the fix works, and one more DOM-view bug.
The causal story was wrong, and it mattered. I wrote that a self-prefetch returns a near-empty fragment anchored at the page's own boundary, and therefore that not prefetching the current page was the cure. Verified against the real app: requesting /docs/client-router with the full have a late self-prefetch sends returns 46,195 bytes anchored at /docs:/docs, byte-identical to a legitimate sibling prefetch. The server short-circuits on LAYOUT segments and deliberately ignores the page's own boundary entry. So that fragment applies fine from a sibling /docs page and fails only from outside /docs, which means the ANCHOR CHECK is the cure and the current-page guard is not. Worse, the guard does not touch the other producer at all: hover a sibling link, never click it, leave the section, click it. Same poison. That path now has its own test, since it is the one that proves which guard does the work.
And the anchor check was still reading the wrong DOM. applyOptimisticLoading deletes everything between its slot's markers, nested boundaries included, and runs before the fetch. On an app whose only loading.{js,ts} is at the root, buildHaveHeader() afterwards reports just the root, so every deeper anchor looks dead and NO prefetch is consumable, permanently, across that whole app. Fixed by capturing the keys before the skeleton deletes them, carried on the state that already threads to fetchAndApply.
Two smaller ones inline, plus one that has no anchorable line left: the have field I added to PrefetchEntry went dead the moment validation moved to the anchor, and its comment claimed it was "kept for diagnostics and tests" when nothing in src or the tests read it. Removed, so the typedef is back to its original shape and no longer appears in the diff.
Findings are anchored at the corrected lines, since the fixes are already pushed.
vivek7405
left a comment
There was a problem hiding this comment.
Third pass, zoomed out to integration and unrelated-behaviour regressions rather than the guards themselves. Clean.
What I checked and what makes it safe:
The new haveKeys plumbing has no staleness path. optimisticState is a fresh local per performNavigation call, handed to fetchAndApply synchronously in the same invocation, so a superseded nav cannot leak its view into a later one. The other applyOptimisticLoading() call site is the form-submit path, which never reaches prefetchTake (that is gated on method === 'GET' && !body && !frameId). Popstate passes null, which is right: the skeleton is skipped on popstate, so the live DOM is the truthful view there, including for the background revalidation that branch fires.
No cost regression. The extra buildHaveHeader() sits after both early returns in applyOptimisticLoading, so an app with no loading.{js,ts} never walks the body an extra time. Apps that do have one pay one comment-walk per navigation, on a path that was already about to fetch.
Nothing else in the repo enumerates router events. packages/core/index.d.ts declares no event map at all, not even for the existing webjs:navigation-error, so there is no typing pattern to follow and adding one only for the new event would be inconsistent. The MCP projects routes, actions, components and check rules, none of which are event surfaces. The three places that DO document events (AGENTS.md, the skill reference, the docs site) are all updated.
Frame navs cannot reach the anchor logic, since prefetchTake is skipped whenever frameId is set, so <webjs-frame> prefetch behaviour is byte-identical to main.
Full sweep on this HEAD: 192 unit, 585 browser across Chromium, Firefox and WebKit, webjs check clean on all four apps, boot check clean with no broken preloads, invariant 11 clean across the whole diff. The real-browser repro rig that reproduced the bug 6-of-24 before the fix stays at 0-of-24.
Nothing outstanding.
|
Reproduction method, for whoever hits the next router timing bug Recording this because the reproduction was the hard part, not the fix, and the technique generalizes. This bug did not reproduce under any of my earlier instrumented runs, including a 15-lap scripted pass against the live site and 120 aggressive randomized clicks locally. The reason is one line: headless Chromium reports What finally reproduced it, in under a minute:
Two measurement traps I fell into and want written down:
The general lesson: when a router bug will not reproduce, suspect the harness is on a different branch of a device-adaptive code path before suspecting the report. |
vivek7405
left a comment
There was a problem hiding this comment.
Went back over the whole diff once more, this time on the paths no earlier pass had touched: the prefetch queue and the in-flight race.
Both hold. drainPrefetchQueue re-enters prefetch() rather than fetching directly, so the current-page guard re-applies at DRAIN time with whatever the location is by then, which is strictly better than before this change (a queued prefetch that has become the current page used to fetch; now it drops). And the race the guard cannot see (a legitimate prefetch in flight when the user navigates onto its target, so it stores an entry for the now-current page) turns out to be harmless specifically BECAUSE of the anchor check: consumed from outside the layout it is discarded, consumed from a sibling it is a valid fragment that applies correctly. Worst case is one cache slot held for the TTL, which is the #1106 waste in race shape, not the #1114 bug.
Also confirmed on this HEAD: a close marker cannot satisfy the anchor regex (the literal requires <!-- directly followed by wj:children:, and a close has the slash in between), the TTL-expiry eviction ordering matches what shipped before, the docs page renders the corrected sections with no stray entities, and AGENTS.md makes no cause-list claim that could drift (only the two enumerating surfaces do, and both carry the full list).
Nothing new. This is the shape I wanted it in: the anchor check does the correctness work, the current-page guard does the economy work, and the event makes the whole class observable so the next one of these does not take four theories to find.
Two fixes that share a cause with everything else in this PR: a rule stated twice drifts, and a loop that stops to wait is a loop nobody runs. Closes #1157. A round is a reading pass over the diff and costs seconds, but the skill never said what NOT to run around one, so sessions re-ran multi-minute suites between rounds and then watched CI on top. Measured on #1156: a single 32-minute gap between two commits, spent running the Node and e2e suites twice each, none of which changed a finding and all of which CI re-ran anyway. Now only the touched test files run per fix, the suites and the CI read happen once after the last clean round, and the text says outright that this changes WHEN they run and never WHETHER, since an earlier attempt at this wording read as permission to skip a layer. Closes #1160. Step 2 of the loop restated the review-posting mechanics and had drifted from the authoritative section on both points: it sent findings through the standalone-comment API rather than one review object, used issue comments for round summaries, and had each finding carry its own disposition. All three are what the posting rules forbid, and the first is how a review on #1115 ended up ungrouped. Step 2 now defers to those rules instead of paraphrasing them, keeping only what is genuinely its own. A pointer cannot drift; a paraphrase will.
* docs: stop a dead review subagent stalling the self-review loop The loop required a fresh subagent reviewer but said nothing about what to do when that reviewer never comes back. Every failure mode (a declined permission prompt, an internal error, a killed background task) ended the same way: the loop stalled silently, or worse, quietly degraded to an inline self-review that then counted as a completed round. That degradation is the expensive part. On PR #1156 two inline rounds reported clean and the first real subagent round that followed found ten findings, three of them genuine bugs in code those rounds had already passed. An inline pass is worse than no review because it looks like one. So: harness status is the only liveness signal (an agent's own prose never is), a failed spawn means the round did not happen, an inline pass is never a round, and a blocked reviewer stops the loop and is reported in the same turn rather than becoming an unreported wait. The reporting shape now says only a round a subagent completed counts toward the total. * docs: fix the liveness check to name a mechanism that works TaskList is the shared to-do list, not a registry of running subagents, so "empty means nothing in flight" reads empty in any session that never called TaskCreate. It reports "dead" identically whether the reviewer is dead or running, which is worse than no check. Issue #1158 specified it, but the premise does not hold. The real signal for the mandated foreground spawn is the Agent call's own result in the same turn, and for a stray background agent it is the harness completion notification or TaskOutput on its name. Also drops the claim that backgrounding is what lets a spawn be declined (a refusal lands on the Agent call either way), and cross-references the new blocked path from the two downstream absolutes that said the loop can only ever end on a clean round. * docs: name a background reviewer's real status signal, not TaskOutput TaskOutput is deprecated and its own description says a local agent's result comes from the Agent tool result, not from the output file (which is the whole subagent transcript). Pointing at it by agent name was the same defect as the TaskList one it replaced: a named mechanism that does not do the job. The harness completion notification arrives on its own and carries the real result, so that is what the file names now. Also: the failed-spawn bullet said "do not report it" next to a bullet demanding you report the blockage, which reads as the exact silent stall this is meant to kill, so it now says do not report it AS A ROUND. And the standalone-review entry point now points at both halves of step 1, since it was sending readers to the liveness rules while skipping the working-tree-safety block that has actually corrupted a checkout. * docs: close the paths that still stall or pass a non-review as a round Tracing the four failure paths through the new wording found daylight in all of them. A spawn that dies after its isolated worktree exists is the likeliest leaker of a stray worktree, and declaring a failed spawn "not a round" had quietly excused it from the repo-health check that fires after each round. The check now keys off the spawn. A killed BACKGROUND agent was routed straight to a blocked loop even though the mandated foreground reviewer had never been tried, so a perfectly reviewable PR could halt on evidence about an agent that was never the reviewer. It now writes that agent off and spawns the real one. A reviewer can return successfully having reviewed nothing ("I could not fetch the diff"), which was neither a finding list nor a literal CLEAN and hit no branch at all. That is now explicitly a failed round, because absence of findings is not a clean round. Retry is pinned: one per round, mechanical failures only, and a fresh round is not a way to buy a second retry. The durable record (draft PR plus the blockage in the body) now applies in every mode rather than only autonomously, since the interactive mode had the weaker record. Failure handling gains the blocked-reviewer case, and the spawned prompt template no longer justifies its git prohibition with a shared working directory the mandated isolation makes false; worktrees sharing one .git directory is the true reason and it survives isolation. * chore: allow the Agent tool so review spawns stop prompting A declined permission prompt is one of the four reviewer failures #1158 names, and it is the only one with a configuration fix rather than a prose one. The repo mandates the self-review loop for every agent that works it, so the permission that lets the loop run belongs in the repo's own committed settings next to the hooks that enforce the rest. Tool-level is the only granularity available, so this allows every Agent spawn rather than only reviewers. * docs: order the blocked-reviewer rules and drop the stale duplicates Read top-down, step 1 told an agent to declare the loop blocked and edit the PR body before it ever reached the bullet granting a retry, so an internal error ended in a blocked PR while still being retryable. Retry now comes first and the ordering is stated outright. The bullets are also no longer referenced by ordinal, since the previous commit added two and left the lead-in saying four, which would have stopped a literal reader one bullet short of the retry rule itself. Three duplicates had drifted from the rules they restate. The working-tree block still opened with the SHARED working directory premise that the prompt template stopped using, the standalone-review pointer still summarized the repo-health check as per-round after it moved to per-spawn, and the Failure handling entry told an autonomous agent to ask the user how to proceed, which AGENTS.md forbids outright. Each now matches its source, and the blocked path no longer demotes a ready PR that another session opened. * docs: give the reviewer a way to say it could not review The returns-without-reviewing rule assumed an output shape the prompt template forbade. The template sanctioned exactly two answers, a finding list or the literal CLEAN, so a reviewer that never fetched the diff had found nothing genuinely wrong and was steered straight into CLEAN, which the loop accepts as converged. The template now defines a BLOCKED sentinel for that case and says outright that reporting CLEAN for a review you could not perform is the worst available outcome. Two rules that admitted the same non-round are fixed with it: the reporting shape tested for a completed call and a subagent result, both of which a non-review satisfies, and the per-round posting rule told a findless round to post a clean review object even when nothing was reviewed. Also: worktree isolation was described as containing every git op, which is the claim the shared .git premise replaces, so it now says isolation covers the working tree while the read-only prohibition covers the refs and config it cannot reach. The two retry paths were unified onto one budget, a recovered failure is reported in the turn rather than through the blocked-PR procedure, the reviewer prompt no longer assumes the branch is checked out locally, and a blocked review is reported to the user instead of being written onto the PR, since a spawn that could not run is session tooling and not a fact about the change. * docs: re-spawn a broken reviewer instead of interrupting the user The blocked path stopped the loop and asked the user how to proceed after a single retry. That is the wrong trade: a spawn that broke mechanically is harness noise, recovering costs seconds, and handing back a half-finished loop costs the review its whole momentum. The user asked for a converged review, not a status update. So a mechanical failure is now re-spawned until one takes, with the prompt adjusted when the reviewer named what it was missing, and varied rather than repeated after a few identical failures. Nothing about this relaxes the rule the block exists for: an inline pass is still never a substitute for keeping the loop moving. A reviewer that genuinely cannot be produced remains the one stopping point, it withholds ready-for-review, and it is reported once at the end rather than as a mid-loop interruption. * docs: keep the loop fast and stop restating the review-posting rules Two fixes that share a cause with everything else in this PR: a rule stated twice drifts, and a loop that stops to wait is a loop nobody runs. Closes #1157. A round is a reading pass over the diff and costs seconds, but the skill never said what NOT to run around one, so sessions re-ran multi-minute suites between rounds and then watched CI on top. Measured on #1156: a single 32-minute gap between two commits, spent running the Node and e2e suites twice each, none of which changed a finding and all of which CI re-ran anyway. Now only the touched test files run per fix, the suites and the CI read happen once after the last clean round, and the text says outright that this changes WHEN they run and never WHETHER, since an earlier attempt at this wording read as permission to skip a layer. Closes #1160. Step 2 of the loop restated the review-posting mechanics and had drifted from the authoritative section on both points: it sent findings through the standalone-comment API rather than one review object, used issue comments for round summaries, and had each finding carry its own disposition. All three are what the posting rules forbid, and the first is how a review on #1115 ended up ungrouped. Step 2 now defers to those rules instead of paraphrasing them, keeping only what is genuinely its own. A pointer cannot drift; a paraphrase will. * docs: resync the three readers of the re-spawn rule Switching the blocked path from one-retry-then-ask to re-spawn-until-it- takes updated the rule and left three places still describing the old one: the failed-spawn bullet told you to report each failure in the turn it happened, the returns-without-reviewing bullet still budgeted a single shared retry, and the Failure handling entry still said to retry once and then ask the user how to proceed. That last one is the same drift this PR keeps finding, in the same file, from my own edit. * docs: shape the loop as a parallel fan-out plus delta verification Measured on this PR's own review: five sequential broad rounds cost 608k subagent tokens and 25 minutes, dominated not by local commands (5.6s of I/O per round) but by each reviewer ingesting a growing payload, and the prior-comments feed alone reached 171 KB by round 5. A parallel fan-out of four narrow lenses then covered more surface in one round's wall clock. So the loop now spawns round 1 as 3-4 narrow reviewers in parallel (correctness, internal consistency, issue satisfaction, mechanical), makes every later round delta-scoped on the fixes, and starves reviewers of context: the diff and touched files only, never prior PR comments, never an exclusion list, with duplicates deduped by the author for free. The same fan-out reviewed this change and its findings are fixed here too: the order lead-in referenced a retry distinction that no longer exists, two bullets pointed at "the bullet below" that was not below, the prompt-spec claimed the template ends with CLEAN when BLOCKED comes after it, the returns-without-reviewing bullet over-claimed the sentinel's coverage, the template never received the branch its spec promised, the reject path still wrote reasons into the PR body, the two blocked-loop summaries omitted the never-reviews case, ready-to-merge was announceable before the deferred suites and CI read, and the no-machinery-tells voice rule contradicted the body's required test plan and dogfood evidence. * docs: convert the per-round machinery to the fan-out shape Adding fan-out and delta rounds to the preamble left the downstream machinery written for one broad sequential reviewer. The delta verify on the previous commit caught all of it: the template still told reviewers to rotate focus per round, a delta round had no prompt shape and would re-read the whole surface, the starve rule forbade the AGENTS.md read the spec required two bullets later, step 3 banned the fan-out re-run the preamble allows, step 4 ended the loop on a clean round 1 that the minimum-two-rounds rule sends to a delta pass, nothing said a fan-out is clean only when every lens is, and the strengthened Contains-verbatim requirement was violated by the file's own template. A fan-out now has explicit round semantics: each lens re-spawns individually without voiding the others, the round completes when every lens returns findings or CLEAN, its findings post as one aggregated review object, and the whole fan-out counts as one round. * docs: close the last two seams in the fan-out conversion The starve rule enumerated a reviewer's permitted context exhaustively and left out the PR title and body that template step 2 has every reviewer fetch, so the rule and the template disagreed about what may be ingested. The body is the author's claims, which the review checks against, so it joins the enumeration rather than leaving the template. And the delta pass a clean fan-out earns had no instantiable scope: with no fix commits, every delta prompt shape pointed at a diff that does not exist. The spec and template now say that pass scopes to the riskiest section of the original diff, matching the minimum-two-rounds rule that created it. * docs: state the delta scope identically in all three places it appears The previous commit widened the delta scope in two of the three places that state it and missed the round-shape bullet that governs the loop, which still scoped every later round to fix commits that do not exist after a clean fan-out. Same for the starve enumeration: expanded in the rule, unexpanded in the prompt-construction bullet built from it, so an agent composing from the bullets starved the reviewer of the title and body the rule had just admitted. The template's charter placeholder also offered no instantiation for the clean-fan-out pass. All three now carry the same rule with the same carve-out. * docs: give the all-rejected round a delta scope too The no-fix-commits fallback was conditioned on "after a clean fan-out", but a round whose findings were ALL rejected also produces no fix commits and is still followed by a mandatory round, so it had no defined scope anywhere: the headline clause was empty and the carve-out did not apply. The condition is now "a round that produced no fix commits", with both causes named, in all four places the rule appears. Also pinned --repo on the title-and-body fetch so a prompt composed from the bullets works when the branch is not checked out locally, matching the template. * docs: trigger step 3's fallback on the mechanism, not two of its causes Findings can be fixed, rejected, or filed, and a round of only rejected and filed findings produces no fix commits either. Step 3's fallback fired only on "every finding rejected", so the filed case executed the main clause against an empty diff. The trigger is now the mechanism itself, the round produced no fix commits, which cannot be under-inclusive whatever dispositions compose it. * docs: state the no-fix-commits condition mechanism-only, everywhere The previous fix put "rejected or filed" in one statement while the three sibling glosses kept the older two-cause enumeration, recreating the drift it was fixing. Cause lists rot every time a disposition is added, so all four statements now name only the mechanism, a round that produced no fix commits, which stays correct whatever composes it. * docs: delta rounds narrow the question, never the evidence "Fed the fix commits' diff plus the paragraphs they touched" read as the reviewer seeing only that, which for a multi-file change would miss a fix's ripple effects somewhere else, the user's exact concern. What made the delta rounds on this PR effective was the opposite split: the fix as the question, the full changed files as evidence, and a mandate to grep every symbol or concept the fix touches across the whole PR surface. That is now what all three statements say. The costly thing a delta round drops is re-auditing the unchanged surface from scratch, the all-pairs reasoning that makes broad rounds slow, not context. * docs: convert step 3 and the intro to the question-evidence model The question-never-evidence rewrite reached three statements of the delta rule and missed the fourth, step 3 of the loop, the one an agent executes literally, which still narrowed the evidence and cited the round-shape rules while saying the opposite of them. The section intro also still promised that later rounds re-read only what changed, priming evidence-scoping two bullets above the rule that forbids it. Both now carry the same model: audit once, then narrow the question while holding the full changed files as evidence. * docs: scale review depth with risk, richest for shipped source WebJs is a framework, so a bug in packages/*/src ships into every end-user app. The round-shape rules now say so: shipped source gets the full shape at its richest (correctness and security lenses, a lens on whether the tests would fail if the change were reverted, a bias toward re-running the fan-out when a fix spans files), while a docs or skill PR may run leaner. The fast-loop rules cut waste, never rigor, and for shipped source the tie breaks toward more review, not less. * docs: single-source the lens list and the fan-out re-run trigger The risk bullet restated two rules and immediately drifted from both: its lens set could not coexist with the fan-out's definitional list, and its spans-files re-run bias contradicted the two "only" statements of the re-run trigger. The lens list is now stated as swappable defaults, the trigger lives in one place (the delta bullet, with the shipped-source lowering inline), and the other statements defer instead of restating. Also converted the last surviving scope-phrased statement of the riskiest-section pass to question form. * docs: adversarial refutation, model diversity, and a deep-review tier Three review-quality upgrades the loop lacked. A shipped-source finding whose fix would be expensive or behavior-changing now gets a refuter before anyone acts on it, because a confident false positive buys a wrong fix plus the delta rounds to unwind it. At least one fan-out lens runs on a different model, since same-family reviewers share the same blind spots. And the riskiest surfaces get a committed deep-review workflow (.claude/workflows/deep-review.js): six finder lenses in parallel, one cross-model, then an adaptive adversarial jury per finding (three refuters for critical, two for major, one for minor, majority kills), returning only what survives refutation, with rejected findings carrying their refuters' reasons. It is the local, no-billing analog of the hosted ultra review, and its confirmed findings feed the normal loop. Syntax validated under the workflow runtime's own wrapping, since a bare module check rejects the top-level return the runtime allows. * docs: actually commit deep-review, generic and safely spawned The previous commit pointed the skill at .claude/workflows/deep-review.js and shipped no such file: .gitignore's .claude/* allowlist has no workflows entry, so git add -A silently skipped it, and the delta round caught the skill sending the riskiest PRs to a workflow that cannot run. The allowlist now unignores .claude/workflows/ and the file is tracked. The workflow is also repo-generic now, per the user: no framework naming, args accept a PR number, owner/repo#N, or { pr, repo }, and when no repo is given each agent detects it from its own cwd via gh. The jury's fail-open stance is documented inline (an empty jury or a tied vote leaves the finding confirmed, never silently dropped). The refuter gate in the skill inherits step 1's spawn rules explicitly (worktree isolation, read-only git, per-spawn repo-health check, re-spawn on failure), and a refuter that cannot be produced means the finding proceeds ungated, since the gate is an optimization and must fail toward treating the finding as real. * docs: pin two distinct models and fix the workflow's rough edges Pinning one lens to fable guaranteed nothing once the session itself runs fable: the pin coincided with the default and every lens ran the same model, making the different-model claim false exactly where the workflow was written to run. Two lenses now pin two distinct models (fable and opus), so at least one always differs from whatever the orchestrating session runs. Also from the delta round: the repo-given SAFETY note told agents to pass --repo to every gh call, but gh api has no --repo flag and errors on it, so the one command needed for reading files at head failed as instructed; the note now splits gh pr from gh api and the fetch line carries the repo in its URL path. And the clean-pass return now has the same shape as the findings return, so a caller reading stats never gets undefined on one path. * docs: pin every deep-review lens, half opus and half fable The session default is opus, so inherit-by-default meant four of six lenses ran the author's own model. All six now pin explicitly: opus for correctness, blast-radius, and tests; fable for security, invariants-docs, and fresh-eyes. Two model families always read the diff, deterministically, whatever the session runs. * docs: let a scout propose dynamic lenses per PR in deep-review Six fixed lenses cover the categories every PR shares, but the angles a specific PR earns depend on what it actually does: concurrency for async coordination, wire compatibility for a serializer change, migration paths for a schema move. A new Scope phase runs one scout that reads the diff, is told the fixed six so it never duplicates them, and proposes zero to six additional lens charters, zero being a fine answer for a PR the fixed set already covers. Dynamic lenses alternate between fable and opus by index, run in the same Find fan-out, and report through the same dedup and jury pipeline. A caller can also pass explicit lenses via args, which skips the scout. Stats now name the dynamic lenses used so a run's shape is auditable from its result. * docs: deep-review is round 1 for every task, no risk bifurcation The user directed a uniform rule: every task entering the review loop starts with the deep-review workflow, whatever the work touches. The scout removed the last advantage the ad-hoc fan-out held (per-PR lens tailoring), the workflow's cost self-scales because juries convene per finding, and a uniform rule cannot be misjudged the way a by-risk bifurcation can. The fan-out machinery, the lens-swapping guidance, and the match-depth-to-risk bullet are deleted rather than kept alongside, since duplicated review rules drifting apart is this file's documented disease. The refuter gate survives scoped to delta-round findings, whose reports are the only ones that now arrive without a jury behind them. Two rules added at the user's direction: deep-review re-runs in full when the PR's scope changes after round 1 (new work folded in, a pivot, an issue absorbed mid-review), because a delta round verifies fixes to an audited surface and cannot stand in for the audit of a surface round 1 never saw. And a standalone "review the PR" request always starts with a fresh deep-review, regardless of prior cycles, because the ask itself says the existing trail is not trusted or not current. The trivial-change skip is unchanged: a one-line typo still enters no loop at all. * docs: convert the per-round machinery to a workflow-shaped round 1 The unification made round 1 a Workflow run and left the machinery written for Agent-tool spawns, the same seam the fan-out conversion hit. The delta round enumerated all of it. The liveness rules now name the Workflow run's completion notification as round 1's status signal, and the clean-round tests recognize each reviewer's expected result shape, since a deep-review run returns a structured result and never the literal CLEAN the old test demanded. The foreground mandate and prompt spec are scoped to the delta verifier. Working-tree safety states honestly that deep-review's internal agents run on the prompt defense alone, which is exactly why the repo-health check now fires after workflow runs too. Round semantics for a jury-adjudicated round are defined: confirmed findings are the round's findings, jury-rejected ones return for the audit trail only and force nothing, while a delta finding the author rejects still forces a round because the author's own rejection is unadjudicated in a way a jury's is not. A manual review ask overrides the trivial-change skip: when the user asks for a review, they get one. * docs: finish converting the three spawn-era recaps The liveness tail still wrote off every dead background agent as never the mandated reviewer, which stopped being true the moment round 1 became a background Workflow run; it now distinguishes a stray agent from the round's own reviewer and names the right recovery for each. The standalone-review recap still restated the old both-defenses-always scheme its source no longer says, and the Failure handling bullet still spoke only of re-spawning a subagent. All three now match the workflow-shaped machinery they summarize. * docs: async reviewer spawns with a transcript watchdog The foreground mandate rebuilt the exact blind wait this PR exists to kill: a synchronous spawn blocks the whole turn, so a hung reviewer is indistinguishable from a working one for as long as the harness takes to give up, and one delta round that should cost minutes ate 20 that way, unwatchable from inside the blocked call. Reviewers now spawn in the background with a watchdog on the task's transcript file, firing on two minutes of stalled mtime or ten minutes total. The peek it triggers reads harness evidence only, task status and whether the transcript is still growing; the words in a transcript remain an agent's own prose and count for nothing, so the #1158 rule survives intact. A stalled reviewer is stopped and re-spawned, a progressing one is left alone, and a hang past the watchdog joins the failure taxonomy. * docs: watchdog checks the reviewer transcript every 30 seconds * docs: one watchdog for both reviewer kinds, with a locatable path The async flip left one clause still calling the delta verifier foreground, inside the non-negotiable block, which was the one sentence that could rebuild the blind wait. The liveness split also gave the round-1 workflow run no mid-flight signal at all, a blind wait for the longest-running reviewer kind, while the failure taxonomy assumed it had one. Both reviewer kinds now share the same pair of signals: the harness notification on completion, the watchdog's output-file activity probe mid-flight, TaskStop on either task id. And the watchdog's path is now specified rather than implied: taken from the spawn's own tool result or task notification, never guessed, since a guess once aimed a watchdog at its own output file; with no path available it degrades to a hard timer on the same thresholds. * docs: use async and multi-agent features across the four heavy skills An audit of all nine skills (filed as #1161, #1162, #1163, all closed by this PR) found four whose dominant cost is independent work run serially, and five that are correctly sequential (a board query, a write-up, a post gain nothing from agents). start-work: the deferred suites and the CI read launch as one batch of background tasks after the last clean round, with every result collected before anything is reported, so the wall clock is the slowest suite rather than the sum, and the WHEN-not-WHETHER rule is explicitly untouched. Also pinned: reviewers are one-shot, never persistent teammates, because accumulated context is the blind-spot sharing the fresh-reviewer rule exists to kill. file-issue: multi-area grounding fans out as parallel read-only Explore agents, one per concern, with the cold-start test unchanged as the gate. doc-sync and scaffold-sync: surface sweeps fan out as read-only Explore agents while edits stay in the main session, since parallel writers on overlapping docs collide; scaffold verifications run as parallel background boots, preferring the portless in-process harness so runs cannot collide, with distinct --port values when a real listen is unavoidable. * docs: the watchdog's mtime probe is meaningless for isolated agents A worktree-isolated agent's output file in the tasks directory is a static stub whose mtime never moves, so probing it reports a stall for every healthy isolated reviewer, and two were killed mid-review on exactly that misread before the CLEAN verdict arrived from the second. For isolated reviewers the watchdog now degrades to the hard timer alone, and only the hard cap or a killed/errored task status justifies a kill. * docs: parallel investigation and verification in the last two skills Closes #1164: research-record investigations fan out one read-only Explore agent per option or angle, with the writeup and its judgment staying with the caller, and the where-the-record-lands rules untouched. Closes #1165: blog-write delegates the de-dup sweep to one Explore agent returning per-post summaries, and runs the dogfood verification's independent parts as one batch of background tasks, Node and Bun boots in parallel with both still mandatory, every result collected before a post is called verified, and browser-dependent probes kept in the main session. * docs: two watchdog modes, decided by what the output file is The stub caveat was one sentence patched into a paragraph whose every other sentence still described the transcript model: the mtime-fire clause fired on every healthy isolated reviewer, the stalled-gets- stopped and tail-peek sentences contradicted the nothing-softer kill rule, the 30-second cadence was decorative for the mandated shape, the liveness paragraph still promised a mid-flight probe for both kinds, and failure handling kept a stalled transcript as a kill trigger. The watchdog now has two explicit modes: transcript mode (the file grows, mtime probe, stall fires, TaskStop and re-spawn) and stub mode (the permanent healthy state of every isolated reviewer, hard timer alone, kill only on the cap or a killed/errored status), and the liveness promise is corrected from never-a-blind-wait to never-an-UNBOUNDED wait, which is the guarantee that was actually true all along. * docs: the headline promises bounded, not blind-free, like the rules under it * docs: hard-cap deep-review at 24 agents per run The run's worst case was ~41 agents (scout, twelve finders, full juries), which is a token bill nobody chose per review. A whole-run budget now caps it at 24 by default, overridable via maxAgents and clamped 8 to 60, degrading gracefully: the six fixed lenses always run, dynamic lenses trim to what remains after reserving four jury slots, juries allocate severity-first from the leftover, and a finding the budget cannot verify is returned in an unverified list with a log line, never silently dropped, per the no-silent-caps rule. Both return paths carry the unverified list and the effective cap in stats so a run's shape is auditable from its result. * docs: the args comment and error message name every accepted key * docs: one task per git worktree, always, hook-enforced Closes #1166. The worktree rule was conditional on concurrency, with a lone-agent plain-branch carve-out whose premise is unverifiable: an agent cannot know another session will not start mid-task, and this repo runs several a night. The start-work skill's steps 3 and 4 now cut a worktree from origin/main unconditionally (a dirty primary neither blocks nor gets fixed, since it is never edited at all), AGENTS.md states the rule without the carve-out, and the instagram skill roots its own worktree at the primary checkout so it cannot nest under a task worktree. Enforcement is real now rather than fictional: AGENTS.md pointed at .claude/hooks/guard-branch-context.sh, which does not exist in the repo. The new require-worktree-for-edits.sh hook blocks (exit 2) any tracked-file edit in a repo's PRIMARY checkout, judged against the FILE's directory rather than the session cwd, since the harness resets cwd to the primary while edits legitimately target worktree files by absolute path. Untracked and gitignored files stay editable, primary detection is the git-dir/git-common-dir equality invariant, and WEBJS_NO_WORKTREE_GATE=1 is the escape hatch, all covered by seven tests including the counterfactual block case. * fix: the worktree gate now guards subdirectories, not just the root The review ran the hook itself and caught it failing open for every tracked file in a SUBDIRECTORY of the primary checkout: invoked via -C on a subdir, git prints --git-dir absolute and --git-common-dir relative (../../.git), so the raw string compare misclassified the primary as a linked worktree, and the gate guarded only root-level files while the tests stayed green by testing exactly those. Both paths now resolve through --path-format=absolute, the test repo carries a nested tracked file whose block case fails against the old hook, and the escape-hatch env var is pinned off in non-bypass tests so an inherited value cannot flip the assertions. Also from the round: the instagram recipe roots every path (mkdir, cp, cd, teardown) at one captured PRIMARY variable instead of half at the primary and half at the session cwd; the start-work skill's three surviving checkout-in-place passages now describe the worktree model, where HEAD-on-main is the primary's healthy state and the task worktree is the only holder of its branch; and AGENTS.md's autonomous-mode line cuts a worktree from origin/main instead of branching in place with the retired feature/ prefix. * docs: teardown re-derives its variables, recovery anchors its checkout The instagram teardown's rooting was illusory: Step 4's user confirmation sits between setup and teardown, shell state does not survive across tool calls, and git -C "" silently degrades to cwd-relative, so the teardown was exactly the unrooted form the prior round flagged. It now re-derives PRIMARY and BR at the top of its own block, and the derivation swaps awk for sed since awk splits a path containing spaces. The start-work recovery command anchors its checkout to the task worktree: run bare from the primary it would succeed and park the PRIMARY on the feature branch, because a detached worktree no longer holds the branch and the git-refuses safety net does not apply. * docs: cap fable-pinned reviewers at three per deep-review run Dynamic lenses alternated fable and opus by index, so a full run could pin six reviewers to fable. A MAX_FABLE budget now caps the pinned fable count across the whole run: the three fixed fable lenses (security, invariants-docs, fresh-eyes) spend it, and every dynamic lens takes opus. Model diversity is unchanged, since both families still always read the diff, and stats report the effective fable count so a run's split is auditable from its result.
Closes #1114
Closes #1106
Summary
Clicking links on webjs.dev intermittently showed a loading spinner in the browser tab and flashed the entire document, navbar included. A tab throbber means a main-frame document load, and the client router was doing one: it consumed a cached prefetch fragment, found no shared boundary to swap into, and correctly degraded to
location.href(the #1015 integrity fallback). Captured cause on every failure, via the event this PR adds:no-shared-boundary.The mechanism. A prefetched fragment is a reduced response. The request carries
X-Webjs-Have(the layout boundaries the client already holds) and the server returns only the divergent part, starting at the deepest LAYOUT boundary it short-circuited on. That starting boundary is the fragment's anchor, and the fragment applies to any live DOM that still offers it with the same route-key. Prefetching any/docspage while the client already holds the docs layout therefore yields a/docs:/docs-anchored fragment. From a sibling/docspage it applies fine. From outside/docsit applies nowhere, and the router full-loads.Two ordinary interactions leave one of those cached at the wrong moment:
The entry then sits under a 30 s TTL, hovering again does not refresh it (the freshness check skips a not-yet-expired entry), and a reload clears the in-page cache. So the reported cycle alternated clean and flashing laps: the "sometimes".
The fix.
prefetchTakevalidates the fragment's anchor against the live boundaries and discards it when the anchor is gone. A discard costs one round-trip; consuming cost a full document load.prefetch()additionally no-ops for the page the user is already on, which is #1106's wasted duplicate request and removes producer 1, though the anchor check is what actually cures the bug.The anchor is validated against the boundaries captured before the optimistic loading skeleton runs.
applyOptimisticLoadingdeletes everything between its slot's markers, nested boundaries included, and runs before the fetch, so on an app whose onlyloading.{js,ts}sits at the root the live view collapses to/:/and every deeper anchor would read as dead: no prefetch consumable, permanently, across that whole app.The diagnostic, kept as a permanent surface
devWarnFallbackbecomesreportFallback, dispatchingwebjs:navigation-fallbackondocumentin all environments with{ cause, href, willReload }, and keeping the dev console warning. Every path that can calllocation.hrefnow reports first, including the two deploy-mismatch cases andhandleNavigationError's last resort, which previously had no diagnostic at all.This is not incidental. The degradation was production-silent, which is why the bug survived two wrong attributions before the real cause was found.
willReloadseparates a click that became a document load from a background op that was merely dropped, which is most of the diagnostic value.Test plan
packages/core/test/routing/router-client.test.js):prefetchAnchorincluding the no-marker degrade-to-permissive path; a table-drivenprefetchTakeover root-anchored-still-live, deep-anchored-gone, deep-anchored-still-live; the current-page guard and its path+search keying; and the event'scause/href/willReloadon both a reloading and a non-reloading degradation. 192 pass.packages/core/test/routing/browser/prefetch-stale-context.test.js, new, 6 tests): both poison producers, both anchor directions, and the root-level-loading.tsinteraction (asserting the live DOM really does hide the deeper boundary while the captured view still carries it). Full suite 585 pass on Chromium, Firefox and WebKit.hover: none, so rigs without the pointer shim never run intent prefetch, which is why this survived every earlier instrumented run.webjs checkclean on all four apps. Dogfood: blog e2e, plus website / docs / ui-website boot with no broken preloads.Docs
Updated:
AGENTS.md(client-navigation paragraph), the skill'sclient-router-and-streamingreference (prefetch section plus a new observing section), andwebsite/app/docs/client-router(anchor validation plus the event, with the full cause list). Scaffold: N/A by propagation, sincecreate.jscopies the canonical skill at create time. Type surface: N/A,packages/core/index.d.tsdeclares no event map at all, not even for the existingwebjs:navigation-error. Version bump: N/A, deferred to the next release decision.Review notes
Three rounds; the last was clean. Rounds 1 and 2 found real problems in my own first attempt, all fixed and visible in the review threads: the validity check was originally a whole-
X-Webjs-Havecomparison that discarded fragments which would have applied (a hit-rate regression on the common mobile flow, and total prefetch failure on any route with aloading.{js,ts}); one browser test passed vacuously; and the causal story I first wrote had the two guards' roles inverted, which is corrected in the code comments, the tests, and all three doc surfaces.